Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't hardcode help text: Autopickup Manager #79564

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Kilvoctu
Copy link
Contributor

@Kilvoctu Kilvoctu commented Feb 6, 2025

Summary

SUMMARY: Interface "Don't hardcode help text: Autopickup Manager"

Purpose of change

Help text should not be hardcoded as, if end user changes their keybinds, it will cause discrepancies in the UI.
This PR targets the autopickup manager and partially addresses #78986

Describe the solution

Pull the currently set keybind for each command, then replace the help text string with a function that enumerates the keybinds. Add one extra line for the help text.
Clean up the help text and leave just the autopickup toggle, and keybinds.

Describe alternatives you've considered

I initially wrote out a string for each line manually (can be seen in initial commit); however, there wasn't enough space, so I gave the help text a third line. Then I realized I can simply implement this the same as my previous PRs, which is cleaner imo.

2nd implementation kept all the help text, but there's still not enough space in the window when accounting for various circumstances as mentioned by moxian.
The help text area could change height to accomodate any amount of help text, but I'm feeling at that point the window is too bloated.

Testing

Game compiles and loads.
Look at autopickup manager on main menu.
Load world and look at autopickup manager.
Press all the command buttons.
Change the keybinds and see the text update.
Press all the new command buttons.

Additional context

screenshot
Screenshot 2025-02-07 000948

@github-actions github-actions bot added the [C++] Changes (can be) made in C++. Previously named `Code` label Feb 6, 2025
@Kilvoctu Kilvoctu force-pushed the helptext-autopickup branch from 348a199 to ce0d758 Compare February 6, 2025 18:58
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions and removed astyled astyled PR, label is assigned by github actions labels Feb 6, 2025
I'll take this opportunity to squash in a minor name change

Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Kilvoctu Kilvoctu force-pushed the helptext-autopickup branch from add3809 to 66e3aa1 Compare February 6, 2025 19:04
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Feb 6, 2025
@moxian
Copy link
Contributor

moxian commented Feb 6, 2025

The mismatch between the actual and displayed keybinds is obviously bad, so unhardcoding these is wonderful!

However, I'm not sure displaying all the keybinding on this screen is the correct approach in the face of translations.
We already have the keybinds listed in the [?] keybinds menu, we kinda don't want to create new translation strings when we have already translated them in the [?] menu (for translators workload sake), and as you note yourself the new layout is a bit tight and will be even tighter in other languages (for example german already has to abbreviate the hell out of them)
20250206-122920-cataclysm-tiles
Even in English I suspect you can get out of bounds when you bind all those things to Insert/Home/PageUp/PageDn/Backspace.

I think it's better to remove all of them (except maybe the [s]witch one) and replace with a [?] Keybindings. (It's a much smaller and less impressive PR, granted, but is still an improvement over the current master IMO)

If you think that displaying all of them here (at the cost of extra translation work) is still better (which is valid!), then at the least we need to size the vertical space dynamically and not hardcode it to 3 lines (but that might be a bigger overhaul)

@Kilvoctu
Copy link
Contributor Author

Kilvoctu commented Feb 6, 2025

I think it's better to remove all of them (except maybe the [s]witch one) and replace with a [?] Keybindings. (It's a much smaller and less impressive PR, granted, but is still an improvement over the current master IMO)

If you think that displaying all of them here (at the cost of extra translation work) is still better (which is valid!), then at the least we need to size the vertical space dynamically and not hardcode it to 3 lines (but that might be a bigger overhaul)

Personally, I have no problems at all with just removing all the help text and leaving a [?] Keybindings. I've done it in the past, too #40236. I like clean UIs, no clutter lol.
I feel like, as you imply, anyone can simply open the keybinding screen if they need help.

There are three other similar "manager" windows (auto notes, safe mode, color), and it'd make things a lot simpler for everyone involved if we do it this way.

@Kilvoctu Kilvoctu marked this pull request as draft February 6, 2025 21:20
display only "switch" and keybindings help
@Kilvoctu
Copy link
Contributor Author

Kilvoctu commented Feb 6, 2025

How does this look 🤔

Using the longest key I can think of for the command here
Screenshot 2025-02-06 164953

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Feb 6, 2025
@moxian
Copy link
Contributor

moxian commented Feb 6, 2025

How does this look

Not very good if I'm being honest.
True [BACKSPACE] Switch is hard to parse, and is somehow losing its context now that it's a bit visually further away from "Autopickup enabled"

Looking at this slimmer version, I feel that we don't need to display the switch keybind anymore (as in: the menu is not getting any less navigable if the keybind mention is gone).
If i hackishly remove it in paint, it looks okay to me:
20250206-151826-ShareX

Also I think if you can manage to put the [?] at the bottom of the screen, it would be better, but I don't feel too strongly about this one.
(And also, that time is probably better spent migrating the whole menu to ImGui 🙃 )

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Feb 6, 2025
@Kilvoctu
Copy link
Contributor Author

Kilvoctu commented Feb 7, 2025

Not a problem, I can remove the unneeded text.

(And also, that time is probably better spent migrating the whole menu to ImGui 🙃 )

Unfortunately I don't have the skill level to migrate windows to ImGui... These minor changes are already tough enough for me.
I don't know when anyone will get around to the ImGui conversions, and I'd really like for these texts to not be hardcoded in the mean time. I want to contribute to ImGui conversions, but each time looking over related code I just don't understand it yet...

@Kilvoctu Kilvoctu marked this pull request as ready for review February 7, 2025 06:15
also accept gh bot suggestion

Co-Authored-By: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Kilvoctu Kilvoctu force-pushed the helptext-autopickup branch from dafd735 to 1aab90b Compare February 7, 2025 06:18
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants